docs(security): review OpenShell v0.0.101 contracts - #8617
Conversation
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis change adds the OpenShell 0.0.101 migration review, a generated child-credential visibility manifest, and Vitest validation for source identity, artifact integrity, security boundaries, runtime configuration, inherited invariants, and qualification selectors. ChangesOpenShell 0.0.101 migration review
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit ffd9425 in the TypeScript / code-coverage/cliThe overall coverage in commit ffd9425 in the Show a code coverage summary of the most impacted files.
Updated |
|
🌿 Preview your docs: https://nvidia-preview-pr-8617.docs.buildwithfern.com/nemoclaw |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/security/openshell-0.0.101-migration-review.md`:
- Around line 22-35: Link the completed migration review to parent epic `#8590` by
adding an explicit reference in the acceptance section of
docs/security/openshell-0.0.101-migration-review.md (lines 22-35), then update
the contract test in test/openshell-0.0.101-migration-review.test.ts (lines
167-186) to assert that `#8590` is present.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0ed40726-7af8-483e-a22f-cf6ebb809c1a
📒 Files selected for processing (4)
ci/source-shape-test-budget.jsondocs/security/openshell-0.0.101-migration-review.mdsrc/lib/actions/sandbox/openshell-child-visible-credentials.v0.0.101.jsontest/openshell-0.0.101-migration-review.test.ts
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 3 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite against this exact revision. Recommended E2E: 2 optional E2E recommendations
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
<!-- markdownlint-disable MD041 --> ## Summary Restore the required root-owned, immutable `0444` mode for the reviewed managed-startup runtime bundle in every managed agent image. The repair uses each final image's existing permission replay so legacy builders remain supported and promotion continues to fail closed. This repair blocks clean trusted main and therefore blocks publication of the draft-only trust-root bootstrap. It does not directly qualify #8616 or #8617. ## Changes - Require the managed-startup runtime bundle to be a regular, non-link file in the OpenClaw, Hermes, and Deep Agents Code final images. - Replay `root:root` ownership and mode `0444`, then require the final numeric stat to be `0:0:444`. - Preserve the reviewed bundle bytes, original builder-stage copy provenance, legacy-builder compatibility, six-variant validation, and aggregate promotion barrier. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [ ] Tests added or updated for changed behavior - [x] Existing tests cover changed behavior — justification: The supported managed-image workflow validator checks regular-file, non-link, and `0:0:444` invariants for all six agent/architecture candidates before aggregate promotion. Focused image-layout and workflow contracts passed for this commit. - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: This restores an existing internal image artifact invariant and changes no command, configuration, default, documented workflow, or supported surface. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Fresh independent correctness/security review of commit `591550ad54cc43b7b18982712b16d40902f41a29` passed all nine security categories with no findings. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `no-docs-needed` - Evidence: Commit 591550a restores the existing root-owned, read-only managed-image runtime artifact contract in three Dockerfiles. It changes no command, configuration, default, documented workflow, or supported surface. - Agent: Codex Desktop <!-- docs-review-head-sha: 591550a --> <!-- docs-review-agents-blob-sha: c4923a3 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run validate:pr` passed after refreshing `origin/main` when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — `47` focused image/layout/workflow tests passed; the targeted Deep Agents Code compatibility contract passed. - [ ] Applicable broad gate passed — Not applicable to this three-Dockerfile permission repair; `npm run checks:repository`, Dockerfile lint, reviewed-bundle verification, and the source-shape audit passed. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) Final resolution still requires the first automatic exact-main managed-image run after external merge to pass all six validation jobs and artifact receipts. No manual E2E run was dispatched. --- Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Strengthened container image validation to ensure the managed startup runtime is present as a regular, non-symlink file. * Confirmed the runtime file is root-owned and has read-only permissions before startup checks complete. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
cv
left a comment
There was a problem hiding this comment.
Reviewed the complete change and the delta since the previous review. No correctness, compatibility, security, test, or documentation blocker found.
Summary
Records the exact OpenShell v0.0.99-to-v0.0.101 and public v0.0.85-to-v0.0.101 migration/security boundaries before any selector changes. It freezes the producer/consumer invariants, qualification selectors, credential-boundary evidence, and one-to-one correction ownership required by the upgrade epic.
Related Issue
Fixes #8599
Parent epic: #8590
Changes
OS101-I01–I16, andOS101-C01–C05ownership.Type of Change
Quality Gates
5279c514079657101e18231b0c49846c16a9af91with no Medium-or-higher finding.Documentation Writer Review
docs-updateddocs/security/openshell-0.0.101-migration-review.md; the writing rules and documentation style were reviewed, and no additional user-facing page is needed because this PR activates no behavior.DGX Station Hardware Evidence
Verification
Signed-off-by:line and every published commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passednpm run docscompleted with 0 errors (two pre-existing Fern warnings); clean Ubuntu root/plugin installs and builds plus source-map validation passed.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: Exact-head GitHub run31256993579passed all eight CLI/integration/E2E-support shards plus build/typecheck, static, audit, installer-integration, plugin, and aggregate checks.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Apurv Kumaria akumaria@nvidia.com
Summary by CodeRabbit
Security
Tests